lib/sysroot: Move staged into deployment list, rework handling

Followup to: https://github.com/ostreedev/ostree/pull/1503
After starting some more work on on this in rpm-ostree, it is
actually simpler if the staged deployment just shows up in the list.

It's effectively opt-in today; down the line we may make it the default,
but I worry about breaking things that e.g. assume they can mutate
the deployment before rebooting and have `/etc` already merged.

There's not that many things in libostree that iterate over the deployment
list.  The biggest change here is around the
`ostree_sysroot_write_deployments_with_options` API.  I initially
tried hard to support a use case like "push a rollback" while retaining
the staged deployment, but everything gets very messy because that
function truly is operating on the bootloader list.

For now what I settled on is to just discard the staged deployment;
down the line we can enhance things.

Where we then have some new gymnastics is around implementing
the finalization; we need to go to some effort to pull the staged
deployment out of the list and mark it as unstaged, and then pass
it down to `write_deployments()`.

Closes: #1539
Approved by: jlebon

diff --git a/src/libostree/ostree-sysroot-cleanup.c b/src/libostree/ostree-sysroot-cleanup.c
index 3698767fc00ae434cabbc003a05f342c8f42066c..1d46222b9a572e788107edde682925db19072838 100644 (file)
--- a/src/libostree/ostree-sysroot-cleanup.c
+++ b/src/libostree/ostree-sysroot-cleanup.c
@@ -308,15 +308,6 @@ cleanup_old_deployments (OstreeSysroot       *self,
       g_hash_table_replace (active_boot_checksums, bootcsum, bootcsum);
     }
 
-  /* And also the staged deployment, if any */
-  if (self->staged_deployment)
-    {
-      char *deployment_path = ostree_sysroot_get_deployment_dirpath (self, self->staged_deployment);
-      g_hash_table_replace (active_deployment_dirs, deployment_path, deployment_path);
-      char *bootcsum = g_strdup (ostree_deployment_get_bootcsum (self->staged_deployment));
-      g_hash_table_replace (active_boot_checksums, bootcsum, bootcsum);
-    }
-
   /* Find all deployment directories, both active and inactive */
   g_autoptr(GPtrArray) all_deployment_dirs = NULL;
   if (!list_all_deployment_directories (self, &all_deployment_dirs,

diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c
index e7dc25d003ce21999d6d804c5c5832965a888e8a..3843a89eb520fdeee30a819cc345fc93cce3fe62 100644 (file)
--- a/src/libostree/ostree-sysroot-deploy.c
+++ b/src/libostree/ostree-sysroot-deploy.c
@@ -2144,6 +2144,38 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot     *self,
 {
   g_assert (self->loaded);
 
+  /* It dramatically simplifies a lot of the logic below if we
+   * drop the staged deployment from both the source deployment list,
+   * as well as the target list.  We don't want to write it to the bootloader
+   * now, which is mostly what this function is concerned with.
+   * In the future we though should probably adapt things to keep it.
+   */
+  if (self->staged_deployment)
+    {
+      if (!glnx_unlinkat (AT_FDCWD, _OSTREE_SYSROOT_RUNSTATE_STAGED, 0, error))
+        return FALSE;
+
+      if (!_ostree_sysroot_rmrf_deployment (self, self->staged_deployment, cancellable, error))
+        return FALSE;
+
+      g_assert (self->staged_deployment == self->deployments->pdata[0]);
+      g_ptr_array_remove_index (self->deployments, 0);
+    }
+  /* First new deployment; we'll see if it's staged */
+  OstreeDeployment *first_new =
+    (new_deployments->len > 0 ? new_deployments->pdata[0] : NULL);
+  g_autoptr(GPtrArray) new_deployments_copy = NULL;
+  if (first_new && ostree_deployment_is_staged (first_new))
+    {
+      g_assert_cmpint (new_deployments->len, >, 0);
+      new_deployments_copy = g_ptr_array_sized_new (new_deployments->len - 1);
+      for (guint i = 1; i < new_deployments->len; i++)
+        g_ptr_array_add (new_deployments_copy, new_deployments->pdata[i]);
+    }
+  else
+    new_deployments_copy = g_ptr_array_ref (new_deployments);
+  new_deployments = new_deployments_copy;
+
   /* Assign a bootserial to each new deployment.
    */
   assign_bootserials (new_deployments);
@@ -2162,6 +2194,8 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot     *self,
       for (guint i = 0; i < new_deployments->len; i++)
         {
           OstreeDeployment *cur_deploy = self->deployments->pdata[i];
+          if (ostree_deployment_is_staged (cur_deploy))
+            continue;
           OstreeDeployment *new_deploy = new_deployments->pdata[i];
           if (!deployment_bootconfigs_equal (cur_deploy, new_deploy))
             {
@@ -2185,12 +2219,12 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot     *self,
   for (guint i = 0; i < new_deployments->len; i++)
     {
       OstreeDeployment *deployment = new_deployments->pdata[i];
-      g_autoptr(GFile) deployment_root = NULL;
+      g_assert (!ostree_deployment_is_staged (deployment));
 
       if (deployment == self->booted_deployment)
         found_booted_deployment = TRUE;
 
-      deployment_root = ostree_sysroot_get_deployment_directory (self, deployment);
+      g_autoptr(GFile) deployment_root = ostree_sysroot_get_deployment_directory (self, deployment);
       if (!g_file_query_exists (deployment_root, NULL))
         return glnx_throw (error, "Unable to find expected deployment root: %s",
                            gs_file_get_path_cached (deployment_root));
@@ -2672,7 +2706,10 @@ ostree_sysroot_stage_tree (OstreeSysroot     *self,
         return FALSE;
     }
 
-  if (!_ostree_sysroot_reload_staged (self, error))
+  /* Bump mtime so external processes know something changed, and then reload. */
+  if (!_ostree_sysroot_bump_mtime (self, error))
+    return FALSE;
+  if (!ostree_sysroot_load (self, cancellable, error))
     return FALSE;
 
   return TRUE;
@@ -2716,10 +2753,17 @@ _ostree_sysroot_finalize_staged (OstreeSysroot *self,
                                     cancellable, error))
     return FALSE;
 
+  /* Now, take ownership of the staged state, as normally the API below strips
+   * it out.
+   */
+  g_autoptr(OstreeDeployment) staged = g_steal_pointer (&self->staged_deployment);
+  staged->staged = FALSE;
+  g_ptr_array_remove_index (self->deployments, 0);
+
   /* TODO: Proxy across flags too? */
   OstreeSysrootSimpleWriteDeploymentFlags flags = 0;
-  if (!ostree_sysroot_simple_write_deployment (self, ostree_deployment_get_osname (self->staged_deployment),
-                                               self->staged_deployment, merge_deployment, flags,
+  if (!ostree_sysroot_simple_write_deployment (self, ostree_deployment_get_osname (staged),
+                                               staged, merge_deployment, flags,
                                                cancellable, error))
     return FALSE;
 
@@ -2744,6 +2788,9 @@ ostree_sysroot_deployment_set_kargs (OstreeSysroot     *self,
                                      GCancellable      *cancellable,
                                      GError           **error)
 {
+  /* For now; instead of this do a redeployment */
+  g_assert (!ostree_deployment_is_staged (deployment));
+
   g_autoptr(OstreeDeployment) new_deployment = ostree_deployment_clone (deployment);
   OstreeBootconfigParser *new_bootconfig = ostree_deployment_get_bootconfig (new_deployment);
 

diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c
index 51d513408711e6ccb11cd8cbc6385d9fdacfcb5d..f59f15086b1b842c24bb5f1b8c605267d02e1a0b 100644 (file)
--- a/src/libostree/ostree-sysroot.c
+++ b/src/libostree/ostree-sysroot.c
@@ -740,6 +740,15 @@ compare_deployments_by_boot_loader_version_reversed (gconstpointer     a_pp,
   OstreeBootconfigParser *a_bootconfig = ostree_deployment_get_bootconfig (a);
   OstreeBootconfigParser *b_bootconfig = ostree_deployment_get_bootconfig (b);
 
+  /* Staged deployments are always first */
+  if (ostree_deployment_is_staged (a))
+    {
+      g_assert (!ostree_deployment_is_staged (b));
+      return -1;
+    }
+  else if (ostree_deployment_is_staged (b))
+    return 1;
+
   return compare_boot_loader_configs (a_bootconfig, b_bootconfig);
 }
 
@@ -824,11 +833,16 @@ _ostree_sysroot_reload_staged (OstreeSysroot *self,
       g_variant_dict_lookup (staged_deployment_dict, "kargs", "^a&s", &kargs);
       if (target)
         {
-          self->staged_deployment =
+          g_autoptr(OstreeDeployment) staged =
             _ostree_sysroot_deserialize_deployment_from_variant (target, error);
-          if (!self->staged_deployment)
+          if (!staged)
             return FALSE;
-          _ostree_deployment_set_bootconfig_from_kargs (self->staged_deployment, kargs);
+
+          _ostree_deployment_set_bootconfig_from_kargs (staged, kargs);
+          if (!load_origin (self, staged, NULL, error))
+            return FALSE;
+
+          self->staged_deployment = g_steal_pointer (&staged);
           self->staged_deployment_data = g_steal_pointer (&staged_deployment_data);
           /* We set this flag for ostree_deployment_is_staged() because that API
            * doesn't have access to the sysroot, which currently has the
@@ -938,8 +952,16 @@ ostree_sysroot_load_if_changed (OstreeSysroot  *self,
   if (root_is_ostree_booted && !self->booted_deployment)
     return glnx_throw (error, "Unexpected state: /run/ostree-booted found and in / sysroot but not in a booted deployment");
 
+  if (!_ostree_sysroot_reload_staged (self, error))
+    return FALSE;
+
   /* Ensure the entires are sorted */
   g_ptr_array_sort (deployments, compare_deployments_by_boot_loader_version_reversed);
+
+  /* Staged shows up first */
+  if (self->staged_deployment)
+    g_ptr_array_insert (deployments, 0, g_object_ref (self->staged_deployment));
+
   /* And then set their index variables */
   for (guint i = 0; i < deployments->len; i++)
     {
@@ -947,9 +969,6 @@ ostree_sysroot_load_if_changed (OstreeSysroot  *self,
       ostree_deployment_set_index (deployment, i);
     }
 
-  if (!_ostree_sysroot_reload_staged (self, error))
-    return FALSE;
-
   /* Determine whether we're "physical" or not, the first time we initialize */
   if (!self->loaded)
     {

diff --git a/src/ostree/ot-admin-builtin-status.c b/src/ostree/ot-admin-builtin-status.c
index 55be69942c87e7e020eac7e14c998cce877483af..ca165f451c640fbc1184806a0a29f62394740e81 100644 (file)
--- a/src/ostree/ot-admin-builtin-status.c
+++ b/src/ostree/ot-admin-builtin-status.c
@@ -201,16 +201,6 @@ ot_admin_builtin_status (int argc, char **argv, OstreeCommandInvocation *invocat
     }
   else
     {
-      OstreeDeployment *staged = ostree_sysroot_get_staged_deployment (sysroot);
-      if (staged)
-        {
-          if (!deployment_print_status (sysroot, repo, staged,
-                                        FALSE, FALSE, FALSE,
-                                        cancellable,
-                                        error))
-            return FALSE;
-        }
-
       for (guint i = 0; i < deployments->len; i++)
         {
           OstreeDeployment *deployment = deployments->pdata[i];

diff --git a/tests/installed/destructive/staged-deploy.yml b/tests/installed/destructive/staged-deploy.yml
index bf50467581ffef7841b6feee3b9cd3e0e7b90119..3f4062b2ea1aedb035de7f11e1056fe53ee50804 100644 (file)
--- a/tests/installed/destructive/staged-deploy.yml
+++ b/tests/installed/destructive/staged-deploy.yml
@@ -1,9 +1,11 @@
-# Test the deploy --stage functionality
+# Test the deploy --stage functionality; first, we stage a deployment
+# reboot, and validate that it worked.
 
 - name: Write staged-deploy commit
   shell: |
     ostree --repo=/ostree/repo commit --parent="${commit}" -b staged-deploy --tree=ref="${commit}" --no-bindings
     ostree admin deploy --stage --karg-proc-cmdline --karg=ostreetest=yes staged-deploy
+    test -f /run/ostree/staged-deployment
   environment:
     commit: "{{ rpmostree_status['deployments'][0]['checksum'] }}"
 - include_tasks: ../tasks/reboot.yml
@@ -13,12 +15,44 @@
     journalctl -b "-1" -u ostree-finalize-staged.service | grep -q -e 'Transaction complete'
     # And that we have the new kernel argument
     grep -q -e 'ostreetest=yes' /proc/cmdline
+    # And there should not be a staged deployment
+    test '!' -f /run/ostree/staged-deployment
 - name: Rollback
   shell: rpm-ostree rollback
 - include_tasks: ../tasks/reboot.yml
 - shell: |
-    ostree refs --delete staged-deploy
     rpm-ostree cleanup -rp
 # And now we shouldn't have the kernel commandline entry
 - name: Check we do not have new kernel cmdline entry
   shell: grep -qv -e 'ostreetest=yes' /proc/cmdline
+
+# Ensure we can unstage
+- name: Write staged-deploy commit, then unstage
+  shell: |
+    ostree admin deploy --stage --karg-proc-cmdline --karg=ostreetest=yes staged-deploy
+    ostree admin status > status.txt
+    grep -qFe '(staged)' status.txt
+    test -f /run/ostree/staged-deployment
+    ostree admin undeploy 0
+    ostree admin status > status.txt
+    grep -vqFe '(staged)' status.txt
+    test '!' -f /run/ostree/staged-deployment
+  environment:
+    commit: "{{ rpmostree_status['deployments'][0]['checksum'] }}"
+
+# Staged should be overwritten by non-staged
+- name: Write staged-deploy commit, then unstage
+  shell: |
+    ostree admin deploy --stage --karg-proc-cmdline --karg=ostreetest=yes staged-deploy
+    test -f /run/ostree/staged-deployment
+    ostree --repo=/ostree/repo commit --parent="${commit}" -b nonstaged-deploy --tree=ref="${commit}" --no-bindings
+    ostree admin deploy --karg-proc-cmdline --karg=ostreetest=yes nonstaged-deploy
+    ostree admin status > status.txt
+    grep -vqFe '(staged)' status.txt
+    test '!' -f /run/ostree/staged-deployment
+    ostree admin undeploy 0
+  environment:
+    commit: "{{ rpmostree_status['deployments'][0]['checksum'] }}"
+
+- name: Cleanup refs
+  shell: ostree refs --delete staged-deploy nonstaged-deploy